Remove more deprecated android code and unify openal settings#2776
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request unifies the OpenAL audio renderer settings across platforms by introducing a new OPENAL constant in AppSettings and deprecating platform-specific constants like LWJGL_OPENAL and ANDROID_OPENAL_SOFT. It also removes the com.jme3.view.surfaceview package and simplifies the Android harness by removing the configureSettings and createSettings hooks. Review feedback identifies a functional regression in JmeFragment where example settings are no longer applied, logic inconsistencies and outdated log messages in JmeAndroidSystem, and a potential NullPointerException in JmeDesktopSystem.
|
🖼️ Screenshot tests have failed. The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests. 📄 Where to find the report:
✅ If you did mean to change things: ✨ If you are creating entirely new tests: Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar". See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information Contact @richardTingle (aka richtea) for guidance if required |
There was a problem hiding this comment.
Pull request overview
This PR removes deprecated/unmaintained Android SurfaceView integration and unifies audio renderer selection across desktop and Android by introducing AppSettings.OPENAL (and switching the default audio renderer to it), aiming to align Android app startup patterns with the desktop backend.
Changes:
- Added
AppSettings.OPENAL, deprecated platform-specific OpenAL constants, and switched the defaultAudioRenderersetting toOPENAL. - Updated desktop audio renderer selection to accept the new
OPENALsetting. - Removed the deprecated Android
com.jme3.view.surfaceviewimplementation and simplifiedAndroidHarnessFragmentstartup/configuration hooks; cleaned up Android examples accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| jme3-desktop/src/main/java/com/jme3/system/JmeDesktopSystem.java | Treat AppSettings.OPENAL as a valid audio renderer choice for desktop OpenAL initialization. |
| jme3-core/src/main/java/com/jme3/system/AppSettings.java | Introduce OPENAL, deprecate old constants, and change the default audio renderer to OPENAL. |
| jme3-android/src/main/java/com/jme3/view/surfaceview/package-info.java | Remove deprecated SurfaceView package documentation (package removed). |
| jme3-android/src/main/java/com/jme3/view/surfaceview/OnRendererStarted.java | Remove deprecated SurfaceView lifecycle interface. |
| jme3-android/src/main/java/com/jme3/view/surfaceview/OnRendererCompleted.java | Remove deprecated SurfaceView lifecycle interface. |
| jme3-android/src/main/java/com/jme3/view/surfaceview/OnLayoutDrawn.java | Remove deprecated SurfaceView lifecycle interface. |
| jme3-android/src/main/java/com/jme3/view/surfaceview/OnExceptionThrown.java | Remove deprecated SurfaceView lifecycle interface. |
| jme3-android/src/main/java/com/jme3/view/surfaceview/JmeSurfaceView.java | Remove deprecated/untested SurfaceView-based rendering path. |
| jme3-android/src/main/java/com/jme3/view/package-info.java | Remove deprecated com.jme3.view package documentation. |
| jme3-android/src/main/java/com/jme3/system/android/JmeAndroidSystem.java | Remove unused audio-renderer “type” tracking API. |
| jme3-android/src/main/java/com/jme3/app/AndroidHarnessFragment.java | Remove settings hook methods and rely on the application/default settings pattern. |
| jme3-android-examples/src/main/res/values/strings.xml | Remove strings tied to deleted menu toggles. |
| jme3-android-examples/src/main/res/menu/menu_items.xml | Remove menu items tied to deleted input toggles. |
| jme3-android-examples/src/main/java/org/jmonkeyengine/jme3androidexamples/TestActivity.java | Stop passing removed input-toggle extras; keep verbose logging flag with explicit default. |
| jme3-android-examples/src/main/java/org/jmonkeyengine/jme3androidexamples/MainActivity.java | Remove input-toggle state and menu handling; add DEFAULT_VERBOSE_LOGGING. |
| jme3-android-examples/src/main/java/org/jmonkeyengine/jme3androidexamples/JmeFragment.java | Move settings setup into createApplication() consistent with the new fragment pattern. |
| jme3-android-examples/build.gradle | Update runAndroidExamples task to only pass verbose logging flag (configurable via property). |
Comments suppressed due to low confidence (2)
jme3-core/src/main/java/com/jme3/system/AppSettings.java:273
- The new
@deprecatedtags for the JOGL renderer constants say "Use LWJGL" but don’t reference a specific replacement constant (and include trailing whitespace). Please update the deprecation text to point to the intended replacement(s) via{@link ...}(e.g., a specificLWJGL_OPENGL*constant) and clean up the formatting so generated Javadoc is unambiguous.
/**
* Use JogAmp's JOGL as the display system, with the OpenGL forward compatible profile
* <p>
* N.B: This backend is EXPERIMENTAL
*
* @see AppSettings#setRenderer(java.lang.String)
* @deprecated Use LWJGL
*/
@Deprecated
public static final String JOGL_OPENGL_FORWARD_COMPATIBLE = "JOGL_OPENGL_FORWARD_COMPATIBLE";
/**
* Use JogAmp's JOGL as the display system, with the backward compatible profile
* <p>
* N.B: This backend is EXPERIMENTAL
*
* @see AppSettings#setRenderer(java.lang.String)
* @deprecated Use LWJGL
*/
@Deprecated
public static final String JOGL_OPENGL_BACKWARD_COMPATIBLE = "JOGL_OPENGL_BACKWARD_COMPATIBLE";
jme3-android/src/main/java/com/jme3/app/AndroidHarnessFragment.java:104
createSettings()/configureSettings()hooks were removed, andonCreate()now starts the app immediately aftercreateApplication(). This is a breaking change for existing subclasses overriding those methods, and the class Javadoc still implies subclasses can "optionally customize settings" without explaining the new pattern. Consider keeping the hooks as deprecated pass-throughs for a transition period or update the Javadoc to clearly state that subclasses must callapp.setSettings(...)insidecreateApplication()(or accept the defaults).
System.setProperty(
BufferAllocatorFactory.PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION,
AndroidNativeBufferAllocator.class.getName());
try {
app = createApplication();
app.start();
OGLESContext context = (OGLESContext) app.getContext();
context.setSystemListener(this);
} catch (Exception exception) {
handleError("jME application initialization failed", exception);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @see AppSettings#setAudioRenderer(java.lang.String) | ||
| * @deprecated This audio renderer has too many limitations. | ||
| * use {@link #ANDROID_OPENAL_SOFT} instead. | ||
| * @deprecated This audio renderer has too many limitations. Use {@link #OPENAL} instead.. |
This PR removes the old untested android surface based path and unifies the LWJGL and android OpenAL AppSetting to simply AppSetting.OPENAL, then removes the createSettings in android fragment, that is now useless.
The goal is to let the developer use the same SimpleApplication creation pattern used in the desktop backend